feat: read the engine's unfulfilled-keys report instead of diffing declared vs delivered - #1308

reviews:
  auto_review:
    drafts: true

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Engine → altimate-code, through the real attach path

This runs the CLI's production attach code under a real instance with no model turn: the binding cache, the datamate lookup on PATH, the --version probe, the allowlist lookup, the MCP spawn, the report and the toast are all the real code (bootstrap(dir, …) → beforeTurn(sessionID)). Only two things are not production: the SaaS API is a local fake (it serves both the CLI's endpoints and the engine's, and every call is asserted 200), and the toast sink is captured instead of published to a TUI bus. The engine is the 0.7.2 release candidate: AltimateAI/altimate-mcp-engine#250's head with AltimateAI/altimate-mcp-engine#248 merged in, built locally (datamate --version → 0.7.2), reached through a shim on PATH exactly as a global install would be.

Declared by the workspace: jira (no connection on this machine), vscode-power-user (extension), mcp-ok (a real second MCP server offering only echo), mcp-missing-binary (command absent), retired-integration (not in the catalog).

Result — release candidate (head engine-rc-0.7.2, 6170 ms from beforeTurn to settled outcome)

Toast (variant warning):

1 of 5 declared integration tools available. Declared but not available — no usable connection: jira_search_issues; not offered by the integration: ghost; server failed to start (spawn altimate-e2e-missing-binary ENOENT): whatever; no longer in the catalog: retired_tool.

Settled outcome:

{
  "kind": "attached",
  "available": 1,
  "declared": 5,
  "missing": [
    "jira_search_issues",
    "ghost",
    "whatever",
    "retired_tool"
  ],
  "unfulfilled": [
    {
      "key": "jira_search_issues",
      "integrationId": "jira",
      "reason": "invalid-connection"
    },
    {
      "key": "pu_lineage",
      "integrationId": "vscode-power-user",
      "reason": "no-bridge"
    },
    {
      "key": "ghost",
      "integrationId": "mcp-ok",
      "reason": "unknown-key"
    },
    {
      "key": "whatever",
      "integrationId": "mcp-missing-binary",
      "reason": "spawn-failed",
      "detail": "spawn altimate-e2e-missing-binary ENOENT"
    },
    {
      "key": "retired_tool",
      "integrationId": "retired-integration",
      "reason": "catalog-missing"
    }
  ]
}

• PASS outcome is attached
• PASS declared counted from the allowlist (5 CLI-servable keys)
• PASS missing = every gap the engine reported except no-bridge
• PASS outcome carries the full report incl. the no-bridge entry
• PASS spawn-failed detail carries the engine's error
• PASS exactly one toast, warning
• PASS toast text
• PASS every API call served (no 404)

API calls, in order: GET /skills -> 200, GET /datamates/77/summary -> 200, GET /datamate_integrations/ -> 200, GET /dbt/v3/validate-credentials -> 200, GET /datamates -> 200, GET /datamate_integrations -> 200, GET /mask -> 200, GET /datamate_integrations/custom -> 200, GET /connections -> 200 — the first three are the CLI's, the rest the engine's.

Result — floor negative (engine 0.7.1, headless)

The same run against a 0.7.1 build settles engine-too-old (found 0.7.1) and prints one stderr line:

Workspace "e2e-rc": 5 integration tools need datamate 0.7.2+ (found 0.7.1). Update with: npm i -g @altimateai/datamate@0.7.2

• PASS engine below the floor is refused as engine-too-old
• PASS refusal names the found version and the floor

Not covered

Windows; a real tenant; the TUI rendering of the toast (the text is asserted, the widget is not).

.e2e/run.sh <engine checkout with dist/> out.json            # attach
HEADLESS=1 .e2e/run.sh <0.7.1 engine> out.json too-old      # floor negative

#!/usr/bin/env bash
# usage: .e2e/run.sh <engine-root> <out.json> [EXPECT]
set -uo pipefail
H=$(mktemp -d /tmp/e2e-engine-to-cli-home-XXXXXX)
env -i HOME="$H" PATH="$H/bin:/usr/local/bin:/usr/bin:/bin:$(dirname "$(command -v bun)"):$(dirname "$(command -v node)")" TERM=dumb ALTIMATE_WORKSPACE=1 ${HEADLESS:+ALTIMATE_CODE_HEADLESS=1} ENGINE_ROOT="$1" EXPECT="${3:-}" \
  timeout 180 bun .e2e/engine-to-cli.ts > "$2" 2> "${2%.json}.stderr"
echo "RC=$? HOME=$H"

// Engine → altimate-code, through the CLI's real attach path. Nothing is
// stubbed except the toast sink (so its text can be captured) and the SaaS
// API (served locally). The binding, the `datamate` lookup on PATH, the
// version probe, the allowlist lookup, the MCP spawn and the report all run
// the production code under a real instance — no model turn.
import http from "node:http"
import path from "node:path"
import { mkdtempSync, mkdirSync, writeFileSync, chmodSync } from "node:fs"
import { tmpdir } from "node:os"
import { execFileSync } from "node:child_process"

const engineRoot = process.env["ENGINE_ROOT"]!
const node = Bun.which("node")!
const DATAMATE_ID = 77

// ---- fake SaaS: engine endpoints + the CLI's own ----------------------------
const catalog = [
  { id: "jira", type: "tool", name: "Jira", description: "", url: "", supportsLocalConnectionTest: true, supportsSaasConnectionTest: false,
    config: [{ key: "url", name: "URL", type: "string", required: true }, { key: "email", name: "Email", type: "string", required: true }, { key: "token", name: "Token", type: "string", required: true }],
    tools: [{ key: "jira_search_issues", name: "Search issues" }] },
  { id: "vscode-power-user", type: "extension", name: "Power User for dbt", description: "", url: "", supportsLocalConnectionTest: false, supportsSaasConnectionTest: false, config: [],
    tools: [{ key: "pu_lineage", name: "Lineage" }] },
]
const custom = [
  { id: "mcp-ok", type: "mcp", name: "Echo MCP", description: "", url: "", config: [],
    toolConfig: [{ key: "type", name: "type", type: "string", required: false, value: "stdio" }, { key: "command", name: "command", type: "string", required: true, value: node },
      { key: "arguments", name: "arguments", type: "array", required: false, value: [path.join(import.meta.dir, "../test/mcp/fixtures/echo-mcp-server.mjs")] }],
    tools: [{ key: "echo" }, { key: "ghost" }] },
  { id: "mcp-missing-binary", type: "mcp", name: "Missing MCP", description: "", url: "", config: [],
    toolConfig: [{ key: "type", name: "type", type: "string", required: false, value: "stdio" }, { key: "command", name: "command", type: "string", required: true, value: "altimate-e2e-missing-binary" }],
    tools: [{ key: "whatever" }] },
]
const datamate = {
  id: String(DATAMATE_ID), name: "e2e-rc", description: "", privacy: "private", memory_enabled: false, knowledge_engine_enabled: false, knowledge_bases: [],
  integrations: [
    { id: "jira", type: "tool", name: "Jira", description: "", url: "", tools: [{ key: "jira_search_issues" }] },
    { id: "vscode-power-user", type: "extension", name: "PU", description: "", url: "", tools: [{ key: "pu_lineage" }] },
    { id: "mcp-ok", type: "mcp", name: "Echo MCP", description: "", url: "", tools: [{ key: "echo" }, { key: "ghost" }] },
    { id: "mcp-missing-binary", type: "mcp", name: "Missing MCP", description: "", url: "", tools: [{ key: "whatever" }] },
    { id: "retired-integration", type: "tool", name: "Retired", description: "", url: "", tools: [{ key: "retired_tool" }] },
  ],
}
const hits: string[] = []
const api = http.createServer((req, res) => {
  const url = new URL(req.url ?? "/", "http://x"); const p = url.pathname.replace(/\/+$/, "") || "/"
  const json = (code: number, body?: unknown) => { hits.push(`${req.method} ${url.pathname} -> ${code}`); res.writeHead(code, { "content-type": "application/json" }); res.end(body === undefined ? "" : JSON.stringify(body)) }
  if (p === "/dbt/v3/validate-credentials") return json(200, { ok: true })
  if (p === "/datamates") return json(200, { datamates: [datamate] })
  if (p === `/datamates/${DATAMATE_ID}/summary`) return json(200, { datamate })
  if (p === "/datamate_integrations") return json(200, catalog)
  if (p === "/datamate_integrations/custom") return json(200, { items: custom })
  if (p === "/mask") return json(200, { mask_data: [] })
  if (p === "/connections") return json(200, { connections: [] })
  if (p === `/datamates/${DATAMATE_ID}/knowledge_bases`) return json(200, { knowledge_bases: [] })
  if (p === `/datamates/${DATAMATE_ID}/knowledge_engine_description`) return json(200, {})
  if (p === "/datamates/audit/create_batch") return json(204)
  if (p === "/skills") return json(200, { skills: [] })
  return json(404, { detail: `unhandled ${p}` })
})
await new Promise<void>((r) => api.listen(0, "127.0.0.1", r))
const apiUrl = `http://127.0.0.1:${(api.address() as { port: number }).port}`

// ---- isolated HOME: the wrapper starts this process with HOME already pointing
// at a fresh directory (Bun caches os.homedir() at startup, so setting it here
// would be too late for Global.Path); this script only fills it in.
const home = process.env["HOME"]!
if (!home.includes("e2e-engine-to-cli-home-")) throw new Error(`refusing to run against a real HOME: ${home}`)
mkdirSync(path.join(home, ".altimate"), { recursive: true })
writeFileSync(path.join(home, ".altimate/altimate.json"), JSON.stringify({ altimateUrl: apiUrl, altimateInstanceName: "e2e", altimateApiKey: "e2e-key" }))
writeFileSync(path.join(home, ".altimate/settings.json"), "{}")
writeFileSync(path.join(home, ".altimate/connections.json"), "[]")
// `datamate` on PATH → the engine under test, on node (the published bin is a node shebang script)
const bin = path.join(home, "bin"); mkdirSync(bin)
writeFileSync(path.join(bin, "datamate"), `#!/bin/sh\nexec "${node}" "${path.join(engineRoot, "dist/cli.js")}" "$@"\n`); chmodSync(path.join(bin, "datamate"), 0o755)
process.env["PATH"] = `${bin}:${process.env["PATH"]}`
process.env["ALTIMATE_WORKSPACE"] = "1"
const resolved = Bun.which("datamate")
const engineVersion = execFileSync(resolved!, ["--version"], { encoding: "utf8" }).trim().split("\n").pop()

// a bound project directory
const project = mkdtempSync(path.join(tmpdir(), "e2e-engine-to-cli-project-"))
execFileSync("git", ["init", "-q", project])

// ---- production modules, imported only after the environment is shaped ----
const { bootstrap } = await import("../src/cli/bootstrap")
const { recordApprovedBinding } = await import("../src/altimate/workspace/state")
const { beforeTurn, settledOutcome } = await import("../src/altimate/workspace/engine-overlay")
const { syncInternals } = await import("../src/altimate/workspace/engine-seams")
const toasts: { title: string; message: string; variant: string }[] = []
syncInternals.notify = async (t) => { toasts.push(t) }   // capture only; no TUI bus here
const lines: string[] = []
syncInternals.printLine = (l) => { lines.push(l) }

await recordApprovedBinding(project, { datamateId: DATAMATE_ID, datamateName: "e2e-rc", repoRemote: null, projectPath: project, linkedAt: Date.now() })
const t0 = Date.now()
const result = await bootstrap(project, async () => {
  await beforeTurn("s1")
  return settledOutcome("s1")
})
const elapsedMs = Date.now() - t0
api.close()

const checks: string[] = []
const check = (label: string, ok: boolean) => checks.push(`${ok ? "PASS" : "FAIL"} ${label}`)
const attached = result?.kind === "attached" ? result : undefined
if (process.env["EXPECT"] === "too-old") {
  check("engine below the floor is refused as engine-too-old", result?.kind === "engine-too-old")
  check("refusal names the found version and the floor", lines.concat(toasts.map((t) => t.message)).some((l) => l.includes(engineVersion!) && l.includes("0.7.2")))
} else {
  check("outcome is attached", !!attached)
  check("declared counted from the allowlist (5 CLI-servable keys)", attached?.declared === 5)
  check("missing = every gap the engine reported except no-bridge", JSON.stringify(attached?.missing) === JSON.stringify(["jira_search_issues", "ghost", "whatever", "retired_tool"]))
  check("outcome carries the full report incl. the no-bridge entry", !!attached?.unfulfilled?.some((u) => u.key === "pu_lineage" && u.reason === "no-bridge"))
  check("spawn-failed detail carries the engine's error", /ENOENT/.test(attached?.unfulfilled?.find((u) => u.key === "whatever")?.detail ?? ""))
  check("exactly one toast, warning", toasts.length === 1 && toasts[0].variant === "warning")
  check("toast text", toasts[0]?.message === "1 of 5 declared integration tools available. Declared but not available — no usable connection: jira_search_issues; not offered by the integration: ghost; server failed to start (spawn altimate-e2e-missing-binary ENOENT): whatever; no longer in the catalog: retired_tool.")
  check("every API call served (no 404)", !hits.some((h) => / -> 404$/.test(h)))
}
console.log(JSON.stringify({ engineRoot, home, resolvedDatamate: resolved, engineVersion, elapsedMs, outcome: result, toasts, lines, apiHits: hits, checks, verdict: checks.every((c) => c.startsWith("PASS")) ? "ALL PASS" : "FAILURES" }, null, 2))
process.exit(checks.every((c) => c.startsWith("PASS")) ? 0 : 1)

One more commit on this head, 82531540c — accept numeric integration ids in the engine report. A tenant-created (custom) MCP integration's id arrives from the engine as a number; the parser treated the whole report as malformed over that one field and the attach announced no gaps at all. The id is now taken as a string (unit test added). Found by the real-chain run for the attach-report post (#1310); the engine side stringifies too (AltimateAI/altimate-mcp-engine#248 64e5bb4), so either side alone is enough.

Multi-model review — client half

Reviewed jointly with AltimateAI/altimate-mcp-engine#248 as one feature. The wire contract between the two halves agrees exactly: meta key, all four field names, all six reason spellings, and the empty-array-vs-absent distinction match. parseUnfulfilled's fail-closed behaviour correctly keeps "I don't know" distinct from "nothing missing", and unknown future reasons pass through verbatim (R1). Release sequencing is tracked separately.

Major

1. Tools and their report are not committed atomically across a refresh

src/mcp/catalog.ts:166,187-190 deletes _meta when a listing starts and republishes it page by page, while defs are replaced only after the whole listing succeeds (src/mcp/index.ts:744-748).

Consequences:

• While a refresh is pending, the previous tools stay visible but their report is gone — a turn silently loses its gap line.
• If a later page fails, partial _meta sits beside stale defs.
• reconcile reads the two through Promise.all (engine-overlay.ts:651), so it has no guarantee of a coherent snapshot.

This bites C3 ("the catalog keeps the report across the paths that list tools") on the tools/list_changed refresh path specifically — the report is kept across completed listings, not across a pending one.

Suggested: accumulate { tools, meta } locally and commit them together on success; retain the last good snapshot on failure; expose one snapshot accessor to the overlay.

Minor

2. The headline and the gap line can disagree (raw vs sanitized key space)

declared.keys are raw API tool.key strings (engine-probes.ts:135-137). present comes from MCP.tools() keys, which pass through sanitize = value.replace(/[^a-zA-Z0-9_-]/g, "_") (mcp/catalog.ts:145, mcp/index.ts:1081), and engineToolKeys only strips the prefix — it can't restore the original name (engine-types.ts:149-154).

So a served schema.inspect becomes datamate_schema_inspect → schema_inspect and never matches the raw declaration. The headline at engine-overlay.ts:662 undercounts.

The undercount itself pre-dates this PR — the old served = declared.keys.length - missing.length used the same mismatched comparison. What changes is the presentation: previously the mismatched key was also (wrongly) named in the missing line; now the engine correctly reports nothing, so the toast reads "3 of 5 declared integration tools available." with no gap line at all. Headline and report contradict each other silently.

Suggested: normalize both sides to one key space before the present.has(k) lookup, or derive served from the report rather than from present.

3. describeMissing attributes one integration's error to another

engine-types.ts:285-303 groups solely by reason and uses the group's first non-empty detail for every key in it. Two integrations that both fail spawn-failed — one spawn docker ENOENT, one a bad path — render under a single error. integrationId is carried on every entry and ignored here, so the toast can give actively wrong repair guidance.

Suggested: group by (integrationId, reason), or only show a shared detail when every member's detail is identical.

4. "Last page's _meta" is really "last page that had one"

catalog.ts:166,187-190 clears once when the listing starts, then sets only if (result._meta !== undefined). A listing whose first page carries _meta and whose final page does not retains the first page's value — which doesn't match the stated per-page clearing. Harmless against today's single-page engine response, but ambiguous as a general accessor contract, and test/mcp/catalog-list-meta.test.ts:58 only covers _meta on the final page.

Suggested: pick a rule (last-page-authoritative vs any-page), implement it inside the completed snapshot, and add a first-page-only case.

5. Truncation is not redaction

engine-types.ts truncates detail to 60 chars into the notification, and engine-overlay.ts:685-691 logs the full report. The engine currently emits raw error.message (flagged on the engine PR); until that's bounded and sanitized upstream, sensitive text can appear at the start of a detail and survive truncation.

6. spawn-failed renders as "server failed to start"

REASON_PHRASE maps it that way, but the engine records transport construction, connect and list failures under that reason — so an auth rejection on a running server reads as a startup failure. Worth aligning the phrase with the engine's actual taxonomy.

Verified sound

• MCP.listMeta(DATAMATE_KEY) resolves correctly — the engine is registered under DATAMATE_KEY (engine-overlay.ts:387,560,634).
• C5 holds: _meta is retained per client via WeakMap<Client, …> and read only for datamate; a reconnect creates a new Client, so old metadata can't transfer.
• C2 holds: reportedMissing excludes only no-bridge; unknown-key on an extension key with a connected bridge does count.
• C1 holds: parseUnfulfilled returns undefined on malformed input, so a bad report yields neither missing nor unfulfilled.
• C4's signature change is right — key=reason means a gap whose reason changed re-announces.
• The served change from declared - missing to filter(present.has) is an improvement for the no-bridge case, which the old arithmetic counted as served.

Re-review disposition — 45a8d02c8

Answering the multi-model review (client half), paired with the engine half on AltimateAI/altimate-mcp-engine#248.

Fixed

• Major 1 — tools and report not committed atomically. The catalog now commits _meta only when a listing completes (the last page that carries one wins; a listing with none clears it; a pending or failed listing leaves the previous value), and MCP stores it in State.meta in the same statement as State.defs. The overlay reads both through one accessor, MCP.snapshot(name), which is one synchronous pass over the state — so a refresh landing between two reads can no longer pair one listing's tools with another's report. Tests: "a _meta on the first page is kept when the last page carries none", "a listing that fails part-way leaves the previous _meta standing", plus the existing three.
• Minor 2 — headline and gap line in different key spaces. served and extServed compare declared keys after sanitize, the same transform the MCP layer applies to tool names, so a served tool whose raw key it renamed still counts.
• Minor 3 — one integration's error attributed to another. describeMissing groups by reason AND integration. Test: "two integrations that failed the same way keep their own details".
• Minor 4 — "last page's _meta" semantics. Stated and tested as "the last page that carries one wins" (first-page-only case added).
• Minor 6 — spawn-failed phrase. Now "server could not be started or reached", which covers construction, connect and list failures as the engine records them.

Recorded, not changed

• Minor 5 — truncation is not redaction. The engine half now bounds and redacts detail before it enters _meta (publicDetail on fix: add altimate-core + dbt-integration to build externals #248); the client keeps truncating for the toast and logs what the engine sent.

Verified on this head: typecheck clean; test/mcp, test/altimate/workspace, test/altimate/plugin and the tool-race suite 735 pass. The six failures in headers.test.ts and oauth-auto-connect.test.ts fail identically on the untouched head in this environment (they need a network or a real server) and are not from this change.

@codex review

Scoped review against the claims below (head 45a8d02c8). For each claim, say whether the code holds it, with a concrete failing scenario where it does not. Please do not re-raise the residuals at the end.

Claims

• C1 — parseUnfulfilled fails closed: a malformed or absent report yields undefined (neither missing nor unfulfilled on the outcome), and unknown future reasons pass through verbatim.
• C2 — reportedMissing excludes only no-bridge; every other reason the engine reports is a gap, and an unknown-key on an extension key with a connected bridge counts.
• C3 — The tools of a listing and its _meta are committed together and read together: State.meta[name] is written in the same statement as State.defs[name] on every path that stores a listing, deleted with it on every path that drops one, and MCP.snapshot(name) reads both in one synchronous pass. A pending or failed refresh leaves the last good pair standing.
• C4 — The catalog commits _meta when a listing completes: the last page carrying one wins, a listing with none clears it, and a failed listing leaves the previous value.
• C5 — served and extServed compare declared keys in the sanitised key space, so the headline and the gap line agree on a served tool whatever its raw key.
• C6 — describeMissing groups by reason and integration; a group's detail is only ever one of its own members' details.
• C7 — A gap whose reason changed re-announces (the reasons are in the announcement signature).

Residuals (already accepted)

• R1 — The client truncates detail for the toast; bounding and redaction happen engine-side (AltimateAI/altimate-mcp-engine#248).
• R2 — spawn-failed stays one reason on the wire; the phrase covers construction, connect and list failures.
• R3 — MCP.listMeta(name) remains for callers that need only the report; the overlay uses snapshot.

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

@codex review

Scoped falsification round on the fixes since the last round (45a8d02c8 → 820147aee). For each claim, say whether the code holds it, with a concrete failing scenario where it does not. Please do not re-raise the accepted residuals.

Fix claims

• F1 — Every commit of a listing stores the tools and the _meta that the SAME listing returned (defsWithMeta): on create, in the tools/list_changed handler, and in storeClient on connect and after OAuth. Two overlapping refreshes for one client can no longer commit one listing's tools with the other's report.
• F2 — served and extServed count a declared key only when its sanitised form is in the catalog AND the engine does not report it unfulfilled; colliding raw keys therefore cannot both count.
• F3 — parseUnfulfilled returns undefined for an entry whose detail is present and not a string; a string, an empty string, or an absent detail parses.

Accepted residuals

• R1 — McpCatalog.listMeta(client) still exists as a per-client read for the catalog tests; nothing on the commit path uses it.
• R2 — Truncation of detail for the toast stays client-side; bounding and redaction are engine-side.

@codex review

Final scoped falsification round (round 3 of 3) on the fix since the last round (820147aee → 945210965). For the claim below, say whether the code holds it, with a concrete failing scenario where it does not. Please do not re-raise the accepted residuals.

Fix claim

• G1 — served and extServed are the number of distinct sanitised catalog entries among the declared keys that are present in the catalog and not reported unfulfilled; declarations that collide after sanitising count once, whether or not the engine listed both.

Accepted residuals

• R1 — A colliding pair where the engine reports one key and lists the other counts the listed one (the report is the authority on which the served tool stands for).
• R2 — McpCatalog.listMeta(client) remains as a per-client read for the catalog tests; nothing on the commit path uses it.

Disposition after three Codex rounds — 413fadceb

Three rounds is the cap, so this is the last: the round-3 finding (an ordinary key and an extension key colliding after sanitising were counted in both groups) is fixed in 413fadceb and pinned by a test. No further Codex round; the remaining verification is by hand.

Verified on this head: typecheck clean; test/mcp, test/altimate/workspace, test/altimate/plugin and the tool-race suite pass (the six headers/oauth-auto-connect failures are environmental and identical on the untouched head). Ready for a human review.